Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix jumping to read marker for events without tiles #4830

Closed
wants to merge 1 commit into from

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Jun 25, 2020

It is possible for your read marker to be set to an event without a tile (like a
reaction). We would still render the read marker at the position of those events
in the timeline, even though there's no matching tile, which breaks a core
assumption of jump to read marker path: it assumes that if the read marker node
is present in the DOM, then there must also be an event tile with a scroll token
matching the event ID.

This fixes the situation by no longer attempting to draw a read marker for
events without tiles.

Fixes element-hq/element-web#10975

It is possible for your read marker to be set to an event without a tile (like a
reaction). We would still render the read marker at the position of those events
in the timeline, even though there's no matching tile, which breaks a core
assumption of jump to read marker path: it assumes that if the read marker node
is present in the DOM, then there must also be an event tile with a scroll token
matching the event ID.

This fixes the situation by no longer attempting to draw a read marker for
events without tiles.

Fixes element-hq/element-web#10975
@jryans jryans requested a review from a team June 25, 2020 10:50
@t3chguy
Copy link
Member

t3chguy commented Jun 25, 2020

Won't this mean to green line at all when you jump to unread?

@jryans
Copy link
Collaborator Author

jryans commented Jun 25, 2020

Won't this mean to green line at all when you jump to unread?

Yes, it does mean that if your read marker points to an event without a tile, now you wouldn't have a read marker line when jumping. It's a bit of edge case, since _getLastDisplayedEventIndex is meant to ensure we're always setting the read marker to something visible, but... it could have a bug of its own, or some other client may have last set you to something web considers hidden.

Saying all this out loud though, I suppose it would be better to always have the green line as before, and instead just fix the scrolling code to go to the right place?

@jryans jryans removed the request for review from a team June 26, 2020 10:29
@jryans
Copy link
Collaborator Author

jryans commented Jun 26, 2020

Removing from queue for now for more changes.

@jryans
Copy link
Collaborator Author

jryans commented Jun 30, 2020

Replaced by a more sensible #4860

@jryans jryans closed this Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clicking Jump to first unread message button does nothing
2 participants